[clang] Improve nested name specifier AST representation #147835
Conversation
mizvekov
requested review from
zygoloid,
rjmccall,
cor3ntin,
AaronBallman,
erichkeane and
zyn0217
mizvekov
requested review from
daniel-grumberg,
QuietMisdreavus,
cyndyishida and
Endilll
as code owners
llvmbot
added
clang
…ecializationType (#153646) This was a regression introduced in llvm/llvm-project#147835 Since this regression was never released, there are no release notes. Fixes llvm/llvm-project#153540
|
We started to see a CI failure around the time this PR landed in https://green.lab.llvm.org/job/llvm.org/job/clang-stage2-Rthinlto/1079/ The build hangs in this step (using a stage 2 compiler): This is where the compiler seems to hang: Any ideas what might be going on here? |
|
Out of curiosity. The old |
|
I don't know if this is the same as the other crash report, but we're also seeing assert failures when building chromium. I've minimized the repro to the following program: This fails when running |
This helps establishing the source file and module ownership origins of the dependencies of some piece of code. |
Seems like it is stuck somewhere calculating the linkage and visibility for some type. |
Thanks! However, for the following piece of code struct A;
struct A {};
struct A;
A* p;
|
This fixes a regression reported here #147835 (comment) Since this regression was never released, there are no release notes.
Yes that is correct. The 3rd line is what declaration was found by type lookup at that point, and the canonical declaration has long been established in clang as the first declaration. |
Thanks, this is going to be fixed by #153862 |
…nType (llvm#153646) This was a regression introduced in llvm#147835 Since this regression was never released, there are no release notes. Fixes llvm#153540
…red bindings (#153923) These are implicit vardecls which its type was never written in source code. Don't create a TypeLoc and give it a fake source location. The fake as-written type also didn't match the actual type, which after fixing this gives some unrelated test churn on a CFG dump, since statement printing prefers type source info if thats available. Fixes #153649 This is a regression introduced in #147835 This regression was never released, so no release notes are added.
…for structured bindings (#153923) These are implicit vardecls which its type was never written in source code. Don't create a TypeLoc and give it a fake source location. The fake as-written type also didn't match the actual type, which after fixing this gives some unrelated test churn on a CFG dump, since statement printing prefers type source info if thats available. Fixes llvm/llvm-project#153649 This is a regression introduced in llvm/llvm-project#147835 This regression was never released, so no release notes are added.
| cast<InjectedClassNameType>(Ty)->getDecl()); | ||
| mangleSourceNameWithAbiTags(cast<InjectedClassNameType>(Ty) | ||
| ->getOriginalDecl() | ||
| ->getDefinitionOrSelf()); |
There was a problem hiding this comment.
Is this getDefinitionOrSelf() call necessary here? Shouldn't InjectedClassNameType always refer to the definition where the class name is injected to?
There was a problem hiding this comment.
Not always.
I mean you could certainly attempt to define an InjectedClassNameType whose declaration necessarily is the definition.
This would cause many annoyances, most of them surmountable, as you wouldn't be able to form a type to an arbitrary declaration (which we do offhandedly in error recovery and printing diagnostics), and the type would play by different rules than other tag types.
Though I think at the end, you would come to the problem of demoted definitions, which come up for example when merging definitions from the GMF of different modules. You would have to throw away the ability to represent an InjectedClassNameType pointing to a demoted definition.
There was a problem hiding this comment.
I've caught an example:
template <typename>
struct Tpl;
template <typename>
struct Tpl {
Tpl(const Tpl&) {}
};The injected name from the copy ctor prototype refers to the forward-declaration from the first two lines. Looks a little strange but Ok.
There was a problem hiding this comment.
Are you sure it's not just its canonical type which refers to the first two lines?
There was a problem hiding this comment.
Heh, interesting. getAs function actually returns the canonical type.
There was a problem hiding this comment.
getAs only desugars. This could happen with a TemplateSpecializationType which its canonical type is an InjectedClassNameType, since in that case desugar just returns the canonical type, but that is not the case for that constructor, since its using an InjectedClassNameType as written in source code.
There was a problem hiding this comment.
I've understood: there are specializations of getAs for the leaf non-sugar types:
llvm-project/clang/include/clang/AST/Type.h
Lines 3138 to 3140 in 71925a9
If you call getAs<InjectedClassNameType>() on a Type which is already InjectedClassNameType, you get its canonical type.
There was a problem hiding this comment.
Ah ok, interesting.
There was a problem hiding this comment.
That looks like it needs to change, we probably don't want to do that for sugared leaf types.
There was a problem hiding this comment.
Btw, it is a drawback of templates that you can easily overlook a specialization.
@Xazax-hun this could be the same as #153933 Could you confirm if the hang is limited to assertions enabled llvm builds, and if #153996 is sufficient to fix the problem? |
The comments in this area are confusing me, FWIW: The function is called "get original decl" which implies you get the first declaration seen in the TU, but the data member it returns has a comment saying it may point to any declaration in the redeclaration chain. One of these is wrong, correct? |
Yeah, that is describing what is valid from the point of view of the AST Node. As a user of TagType, you can certainly create one which points to any declaration of an entity, and all of these nodes which point to a declaration of the same entity are the same type. From the point of view of Sema, there are further rules on how these types are created, in normal day-to-day source code parsing, the declaration pointed to by a non-canonical TagType will be the one found by lookup at that point in the program. FWIW the name The difference in behavior is such that The problem with keeping the name is that the behavior change meant that whenever I would rebase the patch, new users of getDecl would have popped up and it would be hard to make sure all uses of it were correct. By changing the name, I get a compilation error which would allow me to inspect and make the necessary changes. As I stated before, once this patch is settled and everyone has had a nice window to rebase their upstream, my plan is to submit another patch renaming getOriginalDecl back to getDecl. |
llvm/llvm-project#147835 changed how some types are printed, so we need to update the expected messages. Since we need to stay compatible with the previous clang version until we roll, accept both messages for now. Bug: 437910658 Change-Id: I087cdaf241c571efef7c17de78328f0118f232ce Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6859821 Reviewed-by: Daniel Cheng <[email protected]> Auto-Submit: Devon Loehr <[email protected]> Commit-Queue: Daniel Cheng <[email protected]> Cr-Commit-Position: refs/heads/main@{#1502917}
This is a major change on how we represent nested name qualifications in the AST.
This patch offers a great performance benefit.
It greatly improves compilation time for stdexec. For one datapoint, for
test_on2.cppin that project, which is the slowest compiling test, this patch improves-ccompilation time by about 7.2%, with the-fsyntax-onlyimprovement being at ~12%.This has great results on compile-time-tracker as well:
This patch also further enables other optimziations in the future, and will reduce the performance impact of template specialization resugaring when that lands.
It has some other miscelaneous drive-by fixes.
About the review: Yes the patch is huge, sorry about that. Part of the reason is that I started by the nested name specifier part, before the ElaboratedType part, but that had a huge performance downside, as ElaboratedType is a big performance hog. I didn't have the steam to go back and change the patch after the fact.
There is also a lot of internal API changes, and it made sense to remove ElaboratedType in one go, versus removing it from one type at a time, as that would present much more churn to the users. Also, the nested name specifier having a different API avoids missing changes related to how prefixes work now, which could make existing code compile but not work.
How to review: The important changes are all in
clang/include/clang/ASTandclang/lib/AST, with also important changes inclang/lib/Sema/TreeTransform.h.The rest and bulk of the changes are mostly consequences of the changes in API.
PS: TagType::getDecl is renamed to
getOriginalDeclin this patch, just for easier to rebasing. I plan to rename it back after this lands.Fixes #136624
Fixes #43179
Fixes #68670
Fixes #92757